-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Intesar C18 lions #91
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent job! Your code overall looks really good! There are some places where there are a lot of empty lines between code. Make sure to remove empty lines that are not needed.
Remember to add commits more often and to add commit messages that are more descriptive. Instead of saying which wave you're completed, add messages like "Added Clothing class" or "Added get_best_by_category function".
from swap_meet.item import Item | ||
class Clothing(Item): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
from swap_meet.item import Item | ||
class Decor(Item): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
class Electronics: | ||
pass | ||
from swap_meet.item import Item | ||
class Electronics(Item): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
def __init__(self,category = "", condition = 0.0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
def __init__(self, inventory = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
def test_removing_not_found_is_false(): | ||
item = "item to remove" | ||
vendor = Vendor( | ||
inventory=["a", "b", "c"] | ||
) | ||
|
||
result = vendor.remove(item) | ||
|
||
assert not result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great assert! It would also be a good idea to check that the inventory length is still 3 and we didn't accidentally remove something.
@@ -34,7 +34,8 @@ def test_get_no_matching_items_by_category(): | |||
|
|||
items = vendor.get_by_category("electronics") | |||
|
|||
raise Exception("Complete this test according to comments below.") | |||
assert len(items) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
assert len(jesse.get_by_category("Clothing")) == 1 | ||
assert len(jesse.get_by_category("Decor")) == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start, but these asserts only look at Jesse's inventory. Also, having the correct number of categories of items does not necessarily mean we have the specific items we are expecting. Instead, it would be good to do something like what you do for test_swap_best_by_category_reordered()
test
@@ -193,8 +207,10 @@ def test_swap_best_by_category_no_match_is_false(): | |||
my_priority="Clothing", | |||
their_priority="Clothing" | |||
) | |||
|
|||
assert not result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start! It would also be a good idea to check that the items in the inventories did not change, like you do for the other test.
assert result == False | ||
assert len(jesse.inventory) == 3 | ||
assert len(tai.inventory) == 3 | ||
assert item_a in tai.inventory | ||
assert item_b in tai.inventory | ||
assert item_c in tai.inventory | ||
assert item_d in jesse.inventory | ||
assert item_e in jesse.inventory | ||
assert item_f in jesse.inventory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
No description provided.